opt: don't add reordered join with extra filters to original memo group #88779

DrewKimball · 2022-09-27T01:10:56Z

The JoinOrderBuilder builds reordered join plans from the bottom up.
It expects filters to be pushed down as far as possible at each step, and
that transitive closure has been calculated over Inner Join equality filters
(e.g. a=b and b=c => a=c). It also reuses the original matched joins
when possible to avoid duplicate work by adding to the original memo groups.

This could previously cause filters to be dropped in the case when the
original join tree did not compute transitive closure and push filters down
as far as possible. More specifically, the JoinOrderBuilder could add new
reordered joins with new filters synthesized and pushed down as far as possible
to an original memo group that didn't have one of those filters. Subsequent
joins would then expect the filter to be part of the memo group, and so it
wouldn't be added later on in the plan. In the rare case when the expression
without the filter was chosen, this could manifest as a dropped filter in the
final plan. This was rare because dropping a filter usually does not produce
a lower-cost plan.

As an example, take this original join tree:

(xy join ab on true) join uv on x = u and a = u;

Here it is possible to sythesize and push down a x = a filter, and so the
JoinOrderBuilder would do this and add it to the group:

group (xy join ab on true), (xy join ab on x = a)

Later joins would use this group as an input, an expect the x = a filter to
be present. If costing happened to choose the first expression in the group,
we would end up choosing a plan like this:

(xy join ab on true) join uv on x = u

Where the a = u filter isn't included in the top-level join because it
would be redundant to add it when x = u and x = a are already present.
This is a bit of a simplification, but is essentially the problem fixed
by this commit.

This commit adds a check to the JoinOrderBuilder to identify cases where
filters (including ones sythesized from the transitive closure) weren't
pushed all the way down in the original join tree. When this is true, none
of the originally matched joins can be reused when reordered joins are
built except for the root join. This solution may perform some duplicate
work when filters aren't pushed down, but it shouldn't matter because this
case is rare (and should be avoided whenever possible).

Fixes #88659

Release note (bug fix): Fixed a bug introduced in 20.2 that could cause
filters to be dropped from a query plan with many joins in rare cases.

cockroach-teamcity · 2022-09-27T01:11:04Z

This change is

mgartner

Great job tracking this down!

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @DrewKimball)

-- commits line 2 at r1:
nit: consider framing this as something other than "pushdown" - see my other comment

-- commits line 15 at r1:
I think an example like you gave in our session today would be helpful - especially if we come back and read this months or years later. Also, I think it'd be helpful to call out explicitly that this happens when a join with a synthesized filter gets added to an existing memo group that has an expression where the synthesized filter was not created (pushed-down). And that this is only possible when that original expression in the group is chosen as part of the optimal plan, which is rare.

pkg/sql/opt/xform/join_order_builder.go line 367 at r1 (raw file):

		// Ensure that if any of the joins did not successfully push filters down as
		// far as possible, all joins apart from the root join will be rebuilt.
		jb.ensurePushdown()

I think framing this as a problem with "pushdown" is a little confusing, even if that is the root cause. It seems like what we are actually checking is that all edges did not synthesize filters not in the original join. So maybe there is a way to rename this and reword the comments here and below? I think you can still mention the pushdown as a cause for when this would happen, but in theory there could be other reasons we end up synthesizing a filter that wasn't in the original join.

pkg/sql/opt/xform/join_order_builder.go line 395 at r1 (raw file):

		e := &jb.edges[i]
		isFilter := len(e.filters) > 0 && e.filters[0].ScalarProps().OuterCols.Contains(3) && e.filters[0].ScalarProps().OuterCols.Contains(9)
		_ = isFilter

nit: isFilter looks like leftover code from debugging

pkg/sql/opt/xform/join_order_builder.go line 695 at r1 (raw file):

		// This inferred filter was not pushed down as far as possible. All joins
		// apart from the root will have to be rebuilt.
		jb.rebuildAllJoins = true

Is this part needed, or is it an optimization to avoid some of the work in ensurePushdown if we can prove we need to rebuild all joins here?

pkg/sql/opt/xform/join_order_builder.go line 1406 at r1 (raw file):

// was already present in the original join tree. If so, enumerating this join
// would be redundant, so it should be skipped.
func (e *edge) joinIsRedundant(jb *JoinOrderBuilder, s1, s2 vertexSet) bool {

nit: might be simpler to make this a method of JoinOrderBuilder instead of edge now.

nit: Add a note explaining that if rebuildAllJoins is true then none of new joins are redundant with original joins.

pkg/sql/opt/xform/testdata/rules/join_order line 2918 at r1 (raw file):

			(t3.a) = (t4.b)
			AND (t2.b) = (t4.a)
			AND (t2.a) = (t4.a);

nit: tabs to spaces

DrewKimball · 2022-09-30T02:43:37Z

bors r+

craig · 2022-09-30T02:43:39Z

👎 Rejected by too few approved reviews

DrewKimball · 2022-09-30T02:45:55Z

Oops, wrong PR.

DrewKimball

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @mgartner)

-- commits line 2 at r1:

Previously, mgartner (Marcus Gartner) wrote…

nit: consider framing this as something other than "pushdown" - see my other comment

Done, let me know what you think.

-- commits line 15 at r1:

Previously, mgartner (Marcus Gartner) wrote…

I think an example like you gave in our session today would be helpful - especially if we come back and read this months or years later. Also, I think it'd be helpful to call out explicitly that this happens when a join with a synthesized filter gets added to an existing memo group that has an expression where the synthesized filter was not created (pushed-down). And that this is only possible when that original expression in the group is chosen as part of the optimal plan, which is rare.

Added an example. Done.

pkg/sql/opt/xform/join_order_builder.go line 367 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I think framing this as a problem with "pushdown" is a little confusing, even if that is the root cause. It seems like what we are actually checking is that all edges did not synthesize filters not in the original join. So maybe there is a way to rename this and reword the comments here and below? I think you can still mention the pushdown as a cause for when this would happen, but in theory there could be other reasons we end up synthesizing a filter that wasn't in the original join.

I've changed the name to validateEdges and expanded the comments. LMK what you think.

pkg/sql/opt/xform/join_order_builder.go line 395 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: isFilter looks like leftover code from debugging

Oops, good catch. Done.

pkg/sql/opt/xform/join_order_builder.go line 695 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Is this part needed, or is it an optimization to avoid some of the work in ensurePushdown if we can prove we need to rebuild all joins here?

It's necessary since we set the op for the edge to the original join to which the filter would have been pushed if it existed in the original join tree, so the applicability check would always succeed. I've added a comment.

pkg/sql/opt/xform/join_order_builder.go line 1406 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: might be simpler to make this a method of JoinOrderBuilder instead of edge now.

nit: Add a note explaining that if rebuildAllJoins is true then none of new joins are redundant with original joins.

Done.

pkg/sql/opt/xform/testdata/rules/join_order line 2918 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: tabs to spaces

Done.

DrewKimball · 2022-09-30T21:33:25Z

TFTR! I know it's a bit tough to wade through this code.

mgartner

Looks great!

Reviewed 1 of 2 files at r2.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball and @mgartner)

-- commits line 15 at r1:

Previously, DrewKimball (Drew Kimball) wrote…

Added an example. Done.

Nice!

-- commits line 16 at r2:
nit: "and so wouldn't add" -> I think you're missing a word or two there?

-- commits line 38 at r2:
nit: "correct" is confusing given that the plan is incorrect, maybe "is essentially the problem fixed by this commit" or something

pkg/sql/opt/xform/join_order_builder.go line 695 at r1 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

It's necessary since we set the op for the edge to the original join to which the filter would have been pushed if it existed in the original join tree, so the applicability check would always succeed. I've added a comment.

I see. So it seems like this is required to fix this exact bug, while validateEdges provides extra safety for other cases that we aren't currently aware of. is that correct?

The `JoinOrderBuilder` builds reordered join plans from the bottom up. It expects filters to be pushed down as far as possible at each step, and that transitive closure has been calculated over Inner Join equality filters (e.g. `a=b` and `b=c` => `a=c`). It also reuses the original matched joins when possible to avoid duplicate work by adding to the original memo groups. This could previously cause filters to be dropped in the case when the original join tree did not compute transitive closure and push filters down as far as possible. More specifically, the `JoinOrderBuilder` could add new reordered joins with new filters synthesized and pushed down as far as possible to an original memo group that didn't have one of those filters. Subsequent joins would then expect the filter to be part of the memo group, and so it wouldn't be added later on in the plan. In the rare case when the expression without the filter was chosen, this could manifest as a dropped filter in the final plan. This was rare because dropping a filter usually does not produce a lower-cost plan. As an example, take this original join tree: ``` (xy join ab on true) join uv on x = u and a = u; ``` Here it is possible to sythesize and push down a `x = a` filter, and so the `JoinOrderBuilder` would do this and add it to the group: ``` group (xy join ab on true), (xy join ab on x = a) ``` Later joins would use this group as an input, an expect the `x = a` filter to be present. If costing happened to choose the first expression in the group, we would end up choosing a plan like this: ``` (xy join ab on true) join uv on x = u ``` Where the `a = u` filter isn't included in the top-level join because it would be redundant to add it when `x = u` and `x = a` are already present. This is a bit of a simplification, but is essentially the problem fixed by this commit. This commit adds a check to the `JoinOrderBuilder` to identify cases where filters (including ones sythesized from the transitive closure) weren't pushed all the way down in the original join tree. When this is true, none of the originally matched joins can be reused when reordered joins are built except for the root join. This solution may perform some duplicate work when filters aren't pushed down, but it shouldn't matter because this case is rare (and should be avoided whenever possible). Fixes cockroachdb#88659 Release note (bug fix): Fixed a bug introduced in 20.2 that could cause filters to be dropped from a query plan with many joins in rare cases.

DrewKimball

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner)

-- commits line 16 at r2:

Previously, mgartner (Marcus Gartner) wrote…

nit: "and so wouldn't add" -> I think you're missing a word or two there?

Done.

-- commits line 38 at r2:

Previously, mgartner (Marcus Gartner) wrote…

nit: "correct" is confusing given that the plan is incorrect, maybe "is essentially the problem fixed by this commit" or something

Done.

pkg/sql/opt/xform/join_order_builder.go line 695 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

I see. So it seems like this is required to fix this exact bug, while validateEdges provides extra safety for other cases that we aren't currently aware of. is that correct?

Exactly.

DrewKimball · 2022-10-01T22:32:53Z

bors r+

craig · 2022-10-01T23:21:15Z

Build failed:

Bazel Essential CI (Cockroach)

DrewKimball · 2022-10-02T02:14:13Z

bors r+

craig · 2022-10-02T04:04:42Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2022-10-02T04:05:01Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 73e8b28 to blathers/backport-release-21.2-88779: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2.x failed. See errors above.

error creating merge commit from 73e8b28 to blathers/backport-release-22.1-88779: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

DrewKimball · 2022-10-04T22:41:17Z

blathers backport 22.2.0

DrewKimball · 2022-11-10T07:54:35Z

blathers backport 22.1

blathers-crl · 2022-11-10T07:54:40Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 73e8b28 to blathers/backport-release-22.1-88779: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1 failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

DrewKimball requested a review from a team as a code owner September 27, 2022 01:10

mgartner self-requested a review September 27, 2022 16:20

DrewKimball added backport-22.2.x labels Sep 27, 2022

mgartner reviewed Sep 29, 2022

View reviewed changes

DrewKimball added the backport-21.2.x label Sep 30, 2022

DrewKimball force-pushed the join-order branch from 6bdb468 to 8d82367 Compare September 30, 2022 21:32

DrewKimball commented Sep 30, 2022

View reviewed changes

DrewKimball changed the title ~~opt: don't add reordered join to group when filters weren't pushed down~~ opt: don't add reordered join with extra filters to original memo group Sep 30, 2022

mgartner approved these changes Sep 30, 2022

View reviewed changes

DrewKimball force-pushed the join-order branch from 8d82367 to 73e8b28 Compare September 30, 2022 21:55

DrewKimball commented Sep 30, 2022

View reviewed changes

craig bot merged commit ff6961e into cockroachdb:master Oct 2, 2022

blathers-crl bot mentioned this pull request Oct 2, 2022

release-22.2: opt: don't add reordered join with extra filters to original memo group #89158

Merged

blathers-crl bot mentioned this pull request Oct 4, 2022

release-22.2.0: opt: don't add reordered join with extra filters to original memo group #89350

Merged

This was referenced Nov 10, 2022

release-22.1: opt: don't add reordered join with extra filters to original memo group #91654

Merged

opt: fix memo cycle caused by join ordering #83875

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

opt: don't add reordered join with extra filters to original memo group #88779

opt: don't add reordered join with extra filters to original memo group #88779

DrewKimball commented Sep 27, 2022 •

edited

Loading

cockroach-teamcity commented Sep 27, 2022

mgartner left a comment

DrewKimball commented Sep 30, 2022

craig bot commented Sep 30, 2022

DrewKimball commented Sep 30, 2022

DrewKimball left a comment

DrewKimball commented Sep 30, 2022

mgartner left a comment

DrewKimball left a comment

DrewKimball commented Oct 1, 2022

craig bot commented Oct 1, 2022

DrewKimball commented Oct 2, 2022

craig bot commented Oct 2, 2022

blathers-crl bot commented Oct 2, 2022

DrewKimball commented Oct 4, 2022

DrewKimball commented Nov 10, 2022

blathers-crl bot commented Nov 10, 2022

opt: don't add reordered join with extra filters to original memo group #88779

opt: don't add reordered join with extra filters to original memo group #88779

Conversation

DrewKimball commented Sep 27, 2022 • edited Loading

cockroach-teamcity commented Sep 27, 2022

mgartner left a comment

Choose a reason for hiding this comment

DrewKimball commented Sep 30, 2022

craig bot commented Sep 30, 2022

DrewKimball commented Sep 30, 2022

DrewKimball left a comment

Choose a reason for hiding this comment

DrewKimball commented Sep 30, 2022

mgartner left a comment

Choose a reason for hiding this comment

DrewKimball left a comment

Choose a reason for hiding this comment

DrewKimball commented Oct 1, 2022

craig bot commented Oct 1, 2022

DrewKimball commented Oct 2, 2022

craig bot commented Oct 2, 2022

blathers-crl bot commented Oct 2, 2022

DrewKimball commented Oct 4, 2022

DrewKimball commented Nov 10, 2022

blathers-crl bot commented Nov 10, 2022

DrewKimball commented Sep 27, 2022 •

edited

Loading